Skip to content

Add safety documentation analyzers - #131484

Open
EgorBo wants to merge 3 commits into
dotnet:mainfrom
EgorBo:egorbo/unsafe-v2-safety-docs
Open

Add safety documentation analyzers#131484
EgorBo wants to merge 3 commits into
dotnet:mainfrom
EgorBo:egorbo/unsafe-v2-safety-docs

Conversation

@EgorBo

@EgorBo EgorBo commented Jul 28, 2026

Copy link
Copy Markdown
Member

This PR prototypes the safety documentation part of the unsafe-v2 migration tooling (tracking issue §5), continuing #131002, #131245 and #131454. Everything here is non-shipping (#if DEBUG) and disabled by default.

Background

Passing an obligation to a caller comes with a responsibility to say what that obligation is, and discharging one comes with a responsibility to say how. IL5005 from #131002 covers the first half: an unsafe member must carry a <safety> XML comment. Two gaps remain.

The first is the other side of the same contract. An unsafe region is where the obligations are actually met, and that reasoning is invisible unless it is written down. The speclet recommends Rust-style // SAFETY comments and LDM 2026-05-27 left "compiler or analyzer?" open, so an analyzer is the natural home.

The second is safe itself. It is a hand-written assertion the compiler cannot verify, and it is arguably the annotation that most needs a recorded audit, yet nothing checks it.

Changes in this PR

  1. 🔍 UnsafeBlockMissingSafetyCommentAnalyzer (IL5009) reports an unsafe block or unsafe(...) expression with no // SAFETY: comment. A region nested inside an already-documented one is not reported, since the outer comment covers it.

  2. 🔧 AddSafetyCommentCodeFixProvider fixes IL5009 by inserting a // SAFETY: TODO stub, matching the /* SAFETY: Audit */ convention Add unsafe modifier migration code fixer #131002 already uses. The stub only marks the region for review; describing the reasoning is the developer's job.

  3. 🔍 SafeModifierMissingJustificationAnalyzer (IL5010) reports an explicit safe modifier with no <safety> documentation, wherever safe can appear: extern members and [LibraryImport] methods, where it claims the native boundary upholds its contract, and fields in explicit or extended layouts, where it claims the overlap cannot be used to type-pun. No code fix, since only a developer can write the justification.

The SAFETY: marker

Recognized by \bSAFETY\s*:, case-sensitive, matched anywhere inside a comment.

Accepted: // SAFETY:, //SAFETY:, // Reads one element. SAFETY: ..., /* SAFETY: ... */, and a marker on an inner line of a block comment. Rejected: // UNSAFETY: and // SAFETYNET: (whole word), // SAFETY concerns are handled elsewhere (no colon), and // safety: or // Safety:.

Case sensitivity is deliberate: it keeps git grep 'SAFETY:' authoritative as an audit tool, at the cost of a mildly confusing report for someone who writes // Safety:. Both that and the leniency about spacing before the colon are pinned by tests, so they are easy to revisit.

Notable case handled

unsafe(...) expressions are newer than the Roslyn these analyzers compile against, so UnsafeExpressionSyntax and SyntaxKind.UnsafeExpression are unavailable at build time even though they exist at run time. Detecting the expression form as "an unsafe keyword followed by (" is wrong: the modifier is also followed by ( on any member whose type is a tuple.

public static unsafe (int, int) GetPair() => default;   // modifier, not a region
int x = unsafe(Read());                                 // the actual expression

That shape is pervasive in the hardware intrinsics (AdvSimd.cs alone has around 15), and every one of them would have been reported as an undocumented unsafe region, with the fixer offering to stamp a // SAFETY: TODO on it. The analyzer instead tests token.Parent is ExpressionSyntax, which is version-safe because ExpressionSyntax predates the feature.

Diagnostics IDs

Just for reference

Known limitations / follow ups

  • IL5009 treats any // SAFETY: comment as sufficient. It cannot judge whether the reasoning is correct, or whether it still applies after the region is edited.
  • The nesting exemption is deliberately coarse: a documented outer region silences every region inside it, even a much later one that may warrant its own note.
  • This branch adds UnsafeMigrationSyntaxHelpers.SafeKeywordKind, which Add unsafe contract consistency code fixes #131454 also adds. Whichever merges second will need a trivial conflict resolution.

Testing

ILLink.RoslynAnalyzer.Tests: 1238 passed. New coverage for both region forms, the accepted and rejected marker spellings, nesting in both directions, unsafe expressions in field initializers where a block is not valid syntax, the tuple-returning modifier shapes above, and safe on both extern members and explicit-layout fields.

Note

This description and the changes in this PR were generated with GitHub Copilot.

EgorBo and others added 2 commits July 28, 2026 14:28
Adds the safety documentation tier of the unsafe-v2 migration tooling.

IL5009 reports an unsafe block or unsafe(...) expression that records no
// SAFETY: comment explaining how its obligations are discharged, and
AddSafetyCommentCodeFixProvider inserts a stub for review. IL5005 already
covers the signature side of the contract; this covers the place where the
obligations are actually met.

IL5010 reports an explicit safe modifier with no <safety> documentation. It
is the symmetric hole to IL5005: safe is a hand-written assertion the
compiler cannot verify, on a native boundary or on a field whose overlap
must not be used to type-pun. No fixer, since only a developer can write the
justification.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
Replace the substring check with a case-sensitive whole-word regex so that
'// UNSAFETY:' and '// SAFETYNET' no longer count as safety comments, and a
marker is only recognized when it is followed by a colon. Keeping the marker
case-sensitive keeps the convention greppable across a code base.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
Copilot AI review requested due to automatic review settings July 28, 2026 18:17
@EgorBo
EgorBo requested a review from sbomer as a code owner July 28, 2026 18:17
@github-actions github-actions Bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Jul 28, 2026
@dotnet-policy-service dotnet-policy-service Bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jul 28, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke, @dotnet/illink
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR extends the ILLink unsafe-v2 migration tooling prototype by adding analyzers and a code fix to enforce and scaffold safety documentation for unsafe regions (// SAFETY:) and explicit safe modifiers (<safety> XML docs). The additions are gated behind #if DEBUG and are disabled by default, aligning with the existing non-shipping migration tooling approach in this area.

Changes:

  • Add UnsafeBlockMissingSafetyCommentAnalyzer (IL5009) to report undocumented unsafe { } blocks and unsafe(...) expressions, with nesting suppression.
  • Add AddSafetyCommentCodeFixProvider to insert a // SAFETY: TODO stub for IL5009.
  • Add SafeModifierMissingJustificationAnalyzer (IL5010) to require <safety> documentation for explicit safe modifiers, plus corresponding resources/tests and diagnostic wiring.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMigrationTestHelpers.cs Enables the new diagnostics as warnings in the analyzer/code-fix test harness.
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeBlockMissingSafetyCommentAnalyzerTests.cs Adds coverage for IL5009 across block/expression forms, marker spellings, and nesting.
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SafeModifierMissingJustificationAnalyzerTests.cs Adds coverage for IL5010 on safe extern and explicit-layout safe fields.
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddSafetyCommentCodeFixTests.cs Verifies the IL5009 code fix inserts // SAFETY: TODO in the expected location and preserves existing trivia.
src/tools/illink/src/ILLink.Shared/SharedStrings.resx Adds title/message strings for IL5009 and IL5010.
src/tools/illink/src/ILLink.Shared/DiagnosticId.cs Introduces new diagnostic IDs and broadens “Safety” categorization to the 5000–5099 range.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs Exposes SafeKeywordKind and adds a helper to identify unsafe(...) expression keywords safely across Roslyn versions.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeBlockMissingSafetyCommentAnalyzer.cs Implements IL5009 detection for unsafe statements and unsafe(...) expressions, including nesting and trivia scanning rules.
src/tools/illink/src/ILLink.RoslynAnalyzer/SafeModifierMissingJustificationAnalyzer.cs Implements IL5010 detection for explicit safe modifiers lacking <safety> docs.
src/tools/illink/src/ILLink.CodeFix/Resources.resx Adds localized title string for the new code fix.
src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs Implements the IL5009 code fix that inserts a // SAFETY: TODO stub above the appropriate syntax node.

Comment thread src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs Outdated
The fixer inserted the comment above all existing leading trivia, which
contradicted its own doc comment and pushed the marker away from the code it
explains. Insert it directly above the target instead, so XML documentation
keeps its place on top and the marker stays adjacent to the region, matching
the Rust convention the marker is modeled on.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
Copilot AI review requested due to automatic review settings July 28, 2026 19:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

src/tools/illink/src/ILLink.CodeFix/AddSafetyCommentCodeFixProvider.cs:12

  • The using ILLink.RoslynAnalyzer; directive is unused in this file and may produce CS8019/IDE0005 warnings (and can become build-breaking if warnings are treated as errors). Remove it to keep the code fix project warning-clean.
using ILLink.CodeFixProvider;
using ILLink.RoslynAnalyzer;
using ILLink.Shared;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants